-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(web-server): Use isbinaryfile for binary file detection #1816
Conversation
Thank you for your contribution. We detected an issue with your commit message though and would ask you kindly to fix it.
Guidelines are available at http://karma-runner.github.io/0.13/dev/git-commit-msg.html and please do not hesitate to ask if you have any questions. This message was auto-generated by https://gitcop.com |
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
CLAs look good, thanks! |
@@ -249,7 +249,8 @@ | |||
"Jon Bretman <[email protected]>", | |||
"Julian Connor <[email protected]>", | |||
"Jurko Gospodnetić <[email protected]>", | |||
"Karl Lindmark <[email protected]>" | |||
"Karl Lindmark <[email protected]>", | |||
"Matthew Amato <[email protected]>" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert this, we generate this list automatically on release via the git log
Thanks, left some comments inline |
Thanks, should I squash this PR back into a single commit after making changes? |
Thank you for your contribution. We detected an issue with your commit message though and would ask you kindly to fix it.
Guidelines are available at http://karma-runner.github.io/0.13/dev/git-commit-msg.html and please do not hesitate to ask if you have any questions. This message was auto-generated by https://gitcop.com |
var fs = require('graceful-fs') | ||
var crypto = require('crypto') | ||
var mm = require('minimatch') | ||
var extensions = require('./binary-extensions.json').extensions | ||
var isBinaryFile = require('isBinaryFile') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like this needs to be downcased to require('isbinaryfile')
(see travis failures)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoops, thanks. Stupid Windows case insensitivity.
Yes for things like this, there should be a single commit when I merge it. |
Thank you for your contribution. We detected an issue with your commit message though and would ask you kindly to fix it.
Guidelines are available at http://karma-runner.github.io/0.13/dev/git-commit-msg.html and please do not hesitate to ask if you have any questions. This message was auto-generated by https://gitcop.com |
The error comes from you using |
Thanks, I realized that already now I'm trying to figure out why tests are still failing. I think there is an issue with the |
On an unrelated note, the
Should I file a new issue for this? I'm on Windows 7, Node 4.2.x LTS |
please file an issue for this on the validate-commit-msg repo |
All of my changes are in and have been squashed into a single commit. I just noticed that two tests are failing on Node 0.10 tests (but tests pass on all other versions), but it looks like Node 0.10 fails in karma master too. I'm not sure why my branch is failing other than possible incompatibility between |
Yes we need 0.10 compat, but those tests are just time sensitive and flaky on travis for that reason. I've retriggered it and it should be going green soon :) |
Hmm looks like this is a real failure on 0.10 now :( if you can take a look later today I'd appreciate it. Otherwise I can check it out tomorrow |
So I ran the tests over 40 times locally under v0.10.41 on Linux Mint 17.3 and everything passes. I have no idea why they failed on travis. |
Nevermind, I'm a moron and forgot to check out my branch after the initial clone on my linux partition. The tests fail every time. |
Delete `lib/binary-extensions.json` and change the preprocessor to use the `isbinaryfile` module instead. Factor nextPreprocessor function creation into a factory function for code clarity. Update preprocessor specs to use binary data when mocking binary files. Closes #1070
Okay, I've addressed all problems and squashed my latest changes into a single commit. This is ready for final review (and hopefully merger). Thanks for the help. |
Thanks this looks good to me :) |
feat(web-server): Use isbinaryfile for binary file detection
Delete
lib/binary-extensions.json
and change the preprocessor to use theisbinaryfile
module. This is a very simple change but there are some whitespace only changes due to theisBinaryFile
function being asynchronous and therefore adding an extra level of indentation, append?w=1
to the PR url to have the diff ignore whitespace.Closes #1070